-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix(source-github): Graceful error handling of invalid credentials when running all sync operations #67584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(source-github): Graceful error handling of invalid credentials when running all sync operations #67584
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
# It would've been nice to instantiate a single client on this authenticator. However, we are checking | ||
# the limits of each token which is associated with a TokenAuthenticator. And each HttpClient can only | ||
# correspond to one authenticator. | ||
self._http_client = HttpClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i had a thought, instantiating this every time we call this method is very costly, we can probably do some sort mapping of authenticators to the respective http client when we instantiate this authenticator so we don't keep building new ones
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just one small comment about the condition for error message.
org_name = message.split("https://api.github.com/orgs/")[1].split("/")[0] | ||
user_message = f'Organization name: "{org_name}" is unknown, "repository" config option should be updated. Please validate your repository config.' | ||
elif "401 Client Error: Unauthorized for url" in message: | ||
elif "401 Client Error: Unauthorized for url" in message or "401. Error: Unauthorized" in message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this condition be more general, for example, like 'Error: Unauthorized' in message and '401' in message
. So we don't need to update this check every time we get new format of the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep makes sense will do!
Deploy preview for airbyte-docs ready! ✅ Preview Built with commit 147520b. |
Fixes: https://github.com/airbytehq/airbyte-internal-issues/issues/14576
What
We had a previous swarm issue that ended up being a customer misconfiguration due to expired or invalid creds. That being said, the error message was particularly unhelpful w/ a bunch of python stack traces. This replace that with far more useful information:
How
The main thing to call out here is that we want to get in the habit of using our dedicated
HttpClient
which gives us better error handling and just default behavior as a whole instead of using therequests
library. Otherwise the rest is pretty straightforwardReview guide
nah
User Impact
none
Can this PR be safely reverted and rolled back?